Skip to content

Fix race conditions in applicationsignals processor - #1758

Merged
sky333999 merged 3 commits into
mainfrom
applicationsignals_fix
Jul 23, 2025
Merged

Fix race conditions in applicationsignals processor#1758
sky333999 merged 3 commits into
mainfrom
applicationsignals_fix

Conversation

@bjrara

@bjrara bjrara commented Jul 3, 2025

Copy link
Copy Markdown
Collaborator

Description of the issue

We noticed there was race conditions within application signals processor which caused CWA to crash with the error stack as follow:

fatal error: concurrent map writes
goroutine 180863 [running]:
internal/runtime/maps.fatal({0x552d503?, 0x22?})
    runtime/panic.go:1058 +0x18
github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals/internal/cardinalitycontrol.(*topKMetrics).Push(0xc000bd56e0, 0x5d2e590?, 0xc00145f780)
    github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals/internal/cardinalitycontrol/metrics_limiter.go:310 +0x1c5
github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals/internal/cardinalitycontrol.(*service).InsertMetricDataToSecondary(0xc000a8e240, 0xc00145f700)
    github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals/internal/cardinalitycontrol/metrics_limiter.go:209 +0xec
github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals/internal/cardinalitycontrol.(*MetricsLimiter).Admit(0xc000ad4050, {0xc000df1fc9, 0x5}, {0xc00081c1c0?, 0xc000df1f08?}, {0xc001156618?, 0xc000df1f08?})
    github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals/internal/cardinalitycontrol/metrics_limiter.go:139 +0x5e7
github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals.(*awsapplicationsignalsprocessor).processMetricAttributes(0xc000548480, {0xc000df1f08?, 0x3a1c660?}, {0xc001156660?, 0xc000df1f08?}, {0xc001156618?, 0xc000df1f08?})
    github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals/processor.go:295 +0xbd3
github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals.(*awsapplicationsignalsprocessor).processMetrics(0xc000548480, {0x5d61d58, 0xc000b75c20}, {0xc000bd5998?, 0xc000df1f08?})
    github.com/aws/amazon-cloudwatch-agent/plugins/processors/awsapplicationsignals/processor.go:146 +0xd3

Description of changes

In this PR, it refactored cardinalitycontrol module to mitigate the race conditions. Meanwhile, it included applicationsignals into the race test.

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Describe what tests you have done.

Requirements

Before commiting your code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make lint

Integration Tests

To run integration tests against this PR, add the ready for testing label.

@bjrara
bjrara requested a review from a team as a code owner July 3, 2025 20:24
@bjrara
bjrara force-pushed the applicationsignals_fix branch 4 times, most recently from 68fb419 to 4922ad9 Compare July 4, 2025 17:31
mxiamxia
mxiamxia reviewed Jul 8, 2025
View reviewed changes
Comment thread plugins/processors/awsapplicationsignals/internal/cardinalitycontrol/metrics_limiter.go Outdated
mxiamxia
mxiamxia previously approved these changes Jul 8, 2025
View reviewed changes
@bjrara
bjrara force-pushed the applicationsignals_fix branch 2 times, most recently from cffc50e to 99945c0 Compare July 8, 2025 20:11
@github-actions

github-actions Bot commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

This PR was marked stale due to lack of activity.

@github-actions github-actions Bot added the Stale label Jul 17, 2025
@bjrara
bjrara force-pushed the applicationsignals_fix branch from 99945c0 to 6c66449 Compare July 18, 2025 16:25
@github-actions github-actions Bot removed the Stale label Jul 19, 2025
the-mann
the-mann approved these changes Jul 22, 2025
View reviewed changes

@the-mann the-mann left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you use sync.Map? my understanding from the docs is that it has pretty specialized use cases

@sky333999
sky333999 merged commit 0ed03d7 into main Jul 23, 2025
17 checks passed
@sky333999
sky333999 deleted the applicationsignals_fix branch July 23, 2025 19:58
@bjrara

bjrara commented Jul 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Why did you use sync.Map? my understanding from the docs is that it has pretty specialized use cases

Thanks for the question. I switched to sync.Map,mainly for simiplicity. We basically need two locks in this component, to avoid handling the locks mistakenly which could lead to issues like deadlock, I will inevitably introduce a map with locking, which is what sync.Map has done.

From performance perspective, we do meet (1) when the entry for a given key is only ever written once but read many times criteria, we only delete entry when it becomes stale.

TravisStark pushed a commit that referenced this pull request Jul 24, 2025
@bjrara @sky333999 @TravisStark
Fix race conditions in applicationsignals processor (#1758)
69d500f 
Co-authored-by: Kaushik Surya <108111936+sky333999@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mxiamxia mxiamxia mxiamxia approved these changes
+2 more reviewers
@the-mann the-mann the-mann approved these changes
@lisguo lisguo lisguo approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bjrara @mxiamxia @the-mann @lisguo @sky333999